fix(reliability): prevent webview postMessage crashes and make dispose idempotent #11313
Conversation
…e idempotent Closes: RooCodeInc#11311 1. postMessageToWebview() now catches rejections from webview.postMessage() so that messages sent after the webview is disposed do not surface as unhandled promise rejections. 2. dispose() is guarded by a _disposed flag so that repeated calls (e.g. during rapid extension deactivation) are no-ops. 3. CloudService mock in ClineProvider.spec.ts updated to include off() — a pre-existing gap exposed by the new dispose test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0xMink
requested review from
cte,
daniel-lxs,
hannesrudolph,
jr and
mrubens
as code owners
dosubot
bot
added
size:M
Re-reviewed the latest commits (e0808ae...948069c). The added
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
Clean, well-scoped PR. The idempotent dispose guard and the postMessage try/catch are both correct and well-tested.
Minor observations (non-blocking):
-
Bare
catch {}inpostMessageToWebview— This silently swallows all errors, not just "webview is disposed" ones. In practice the only thingwebview.postMessage()can throw is a disposed-webview error, so this is fine. If you wanted to be defensive you could log at debug level, but it's not necessary. -
_disposednot checked inpostMessageToWebview— Afterdispose()runs,this.viewis disposed but not nulled out, sopostMessageToWebviewstill attempts the call and relies on the try/catch. An earlyif (this._disposed) returnwould avoid the unnecessary call entirely, but the try/catch is the more robust approach since it also handles external disposal (VS Code closing the webview independently). So the current approach is arguably better. -
Mock fix — Adding
off: vi.fn()to theCloudServicemock is a legitimate gap fix, good catch.
LGTM ✅
Skip the postMessage call entirely when the provider is already disposed, avoiding unnecessary try/catch execution. Added test coverage for this path.
2 participants
Closes: #11311
Summary
dispose(): Added_disposedguard so repeated calls (e.g. during rapid extension deactivation) are no-ops.postMessageToWebview()rejections: Wrappedwebview.postMessage()in try/catch so messages sent after the webview is disposed do not surface as unhandled promise rejections.CloudServicemock to includeoff()— a pre-existing gap exposed by the new dispose test.Test plan
postMessageToWebviewdoes not throw when webview is disposeddispose()is idempotent — second call is a no-opRoadmap alignment: Reliability First
Important
Make
dispose()idempotent and handle promise rejections inpostMessageToWebview()inClineProviderto improve reliability.dispose()inClineProvideridempotent by adding_disposedflag to prevent repeated execution.postMessageToWebview()inClineProviderwith try/catch to handle promise rejections when webview is disposed.postMessageToWebviewto ensure no error is thrown when webview is disposed.dispose()to verify it is idempotent and logs only once.CloudServicemock to includeoff()method inClineProvider.spec.ts.This description was created by
for e0808ae. You can customize this summary. It will automatically update as commits are pushed.